HYPERFLEET-1057 - feat: Fix e2e dockerfile to properly install helm p…#125
HYPERFLEET-1057 - feat: Fix e2e dockerfile to properly install helm p…#125ma-hill wants to merge 1 commit into
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Dockerfile's Helm setup was refactored to bootstrap an explicit temporary Helm home directory at /tmp/helm-home. Rather than relying on default Helm home locations, the build now sets HELM_CACHE_HOME, HELM_CONFIG_HOME, HELM_DATA_HOME, and HELM_PLUGINS environment variables pointing to that temporary directory. The helm-git and helm-diff plugins are installed into this isolated environment using the same pinned version build arguments, and the temporary directory is configured with recursive write permissions. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Dockerfile (3)
32-33:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCWE-250, CWE-732: Runtime stage runs as root.
The runtime stage switches to
USER root(line 32) and never drops privileges. The container executes as UID 0, violating the hardening guideline: "Do not run as root — use USER with a non-root UID." A compromised process gains full container privileges.Add a
USER <non-root-uid>directive before the ENTRYPOINT (after line 86) to drop privileges.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 32 - 33, The Dockerfile currently sets USER root and never drops privileges; before the ENTRYPOINT directive add a non-root USER (e.g., USER 1000 or a named user) to run the runtime stage as a non-root UID, ensure the chosen UID has ownership/permissions on app files and any needed directories (use chown/chmod during build or create a user via groupadd/useradd in the Dockerfile), and verify ENTRYPOINT and any startup scripts are executable by that user so the container no longer runs as root.Source: Coding guidelines
7-7:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBase images not pinned by digest.
Lines 7, 10, 26, and 29 reference base images by tag (including
:lateston line 26), violating guideline#3: "Pin base images by digest (@sha256:...) not just tag." Tags are mutable; a registry compromise or tag retargeting injects a backdoor into the build.Pin each base image to a SHA256 digest:
- Line 7:
registry.access.redhat.com/ubi9/go-toolset@sha256:...- Line 10:
golang:1.25@sha256:...- Line 26:
registry.ci.openshift.org/.../hyperfleet-credential-provider@sha256:...Also applies to: 10-10, 26-26, 29-29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` at line 7, Update the Dockerfile to pin all base images by SHA256 digest instead of mutable tags: replace the ARG BASE_IMAGE value and any FROM/image references (e.g., ARG BASE_IMAGE, the golang image reference, and the hyperfleet-credential-provider image) so they use the registry@sha256:... form rather than a tag or :latest; ensure you obtain the correct sha256 digests from the registries and substitute them in each referenced symbol (BASE_IMAGE and the explicit golang and hyperfleet image references) so every base image is immutably pinned.Source: Coding guidelines
36-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winkubectl binary downloaded without signature verification.
Lines 36-37 download the kubectl binary from
dl.k8s.iowithout verifying a checksum or GPG signature. A compromised CDN or MITM attack injects a backdoored binary.Add checksum verification using the
.sha256file published alongside each kubectl release, or verify the binary's GPG signature before installation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 36 - 37, The Dockerfile RUN step that downloads the kubectl binary (the RUN ... kubectl && chmod +x /usr/local/bin/kubectl line) lacks integrity verification; update this step to fetch the corresponding .sha256 (or GPG signature) for the release (use the same release string from https://dl.k8s.io/release/stable.txt), verify the downloaded kubectl against the checksum (or verify the signed checksum using the Kubernetes release key), and abort (non-zero exit) if verification fails before running chmod +x; ensure the verification uses the exact same URL/version used to download the binary so the process is atomic and secure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile`:
- Line 43: The Dockerfile currently makes /tmp/helm-home world-writable with
"chmod 777 /tmp/helm-home"; change this to use a more secure permission and
ownership: replace the chmod 777 with "chmod 755 /tmp/helm-home" and add a chown
to the non-root test user (e.g., "chown <UID>:<GID> /tmp/helm-home" or use the
existing user name) so only that user can write, or remove explicit chmod and
create the directory after switching to the non-root user; update both
occurrences that touch /tmp/helm-home to follow this pattern.
- Line 40: Replace the unsafe "RUN curl -fsSL
https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 | bash"
pattern with a pinned, verifiable Helm install: choose a specific Helm version,
download the corresponding release tarball or binary directly (not a remote
install script), fetch its published SHA256 (or signature) from the official
release assets, verify the checksum/signature before extracting, and then move
the verified helm binary into the target PATH (or install from a pinned OS
package/RPM/container layer). Update the Dockerfile step that currently uses the
piped script (the RUN curl -fsSL ... | bash line) to perform these explicit
download, verify, extract, and clean-up actions instead.
---
Outside diff comments:
In `@Dockerfile`:
- Around line 32-33: The Dockerfile currently sets USER root and never drops
privileges; before the ENTRYPOINT directive add a non-root USER (e.g., USER 1000
or a named user) to run the runtime stage as a non-root UID, ensure the chosen
UID has ownership/permissions on app files and any needed directories (use
chown/chmod during build or create a user via groupadd/useradd in the
Dockerfile), and verify ENTRYPOINT and any startup scripts are executable by
that user so the container no longer runs as root.
- Line 7: Update the Dockerfile to pin all base images by SHA256 digest instead
of mutable tags: replace the ARG BASE_IMAGE value and any FROM/image references
(e.g., ARG BASE_IMAGE, the golang image reference, and the
hyperfleet-credential-provider image) so they use the registry@sha256:... form
rather than a tag or :latest; ensure you obtain the correct sha256 digests from
the registries and substitute them in each referenced symbol (BASE_IMAGE and the
explicit golang and hyperfleet image references) so every base image is
immutably pinned.
- Around line 36-37: The Dockerfile RUN step that downloads the kubectl binary
(the RUN ... kubectl && chmod +x /usr/local/bin/kubectl line) lacks integrity
verification; update this step to fetch the corresponding .sha256 (or GPG
signature) for the release (use the same release string from
https://dl.k8s.io/release/stable.txt), verify the downloaded kubectl against the
checksum (or verify the signed checksum using the Kubernetes release key), and
abort (non-zero exit) if verification fails before running chmod +x; ensure the
verification uses the exact same URL/version used to download the binary so the
process is atomic and secure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1743b232-a653-40b6-a8c1-ffc24640b1f5
📒 Files selected for processing (1)
Dockerfile
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
ma-hill
force-pushed
the
HYPERFLEET-1057-2
branch
from
75e826c to
ed1ba57
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Dockerfile (2)
32-86:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winContainer still runs as root by default (CWE-250).
USER rootis set and never dropped in the final runtime stage. This violates the hardening requirement and expands blast radius for any compromised tool/plugin/script in the image.As per coding guidelines, “Do not run as root — use USER with a non-root UID.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 32 - 86, The image currently sets USER root and never drops privileges; create a non-root runtime user (e.g., UID 1000, group wheel or nobody) and switch to it before the final ENTRYPOINT so the container does not run as root. Ensure all files and directories used at runtime (references: /usr/local/bin/hyperfleet-e2e, /usr/local/bin/hyperfleet-credential-provider, /e2e, /e2e/configs, /e2e/testdata, /e2e/deploy-scripts, /e2e/env, /e2e/scripts) are owned or accessible by that user (chown/chmod in the Dockerfile during image build) and then add USER <nonroot> right before ENTRYPOINT to enforce the non-root runtime. Maintain existing ENTRYPOINT and CMD but run them under the non-root account.Source: Coding guidelines
7-7:⚠️ Potential issue | 🟠 MajorPin Docker base images to immutable digests (CWE-494 supply-chain risk)
Dockerfile:FROM golang:1.25,FROM registry.ci.openshift.org/ci/hyperfleet-credential-provider:latest,ARG BASE_IMAGE=registry.access.redhat.com/ubi9/go-toolset+FROM ${BASE_IMAGE}images/Dockerfile.platform:FROM registry.access.redhat.com/ubi9/ubi:latestNone are digest-pinned (
@sha256), so rebuilds can silently pull different contents. Pin eachFROMimage by digest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` at line 7, Pin the base images to immutable digests: replace each floating tag with the corresponding `@sha256` digest (e.g., change "FROM golang:1.25", "FROM registry.ci.openshift.org/ci/hyperfleet-credential-provider:latest", the ARG/default "ARG BASE_IMAGE=registry.access.redhat.com/ubi9/go-toolset" and the later "FROM ${BASE_IMAGE}", and "FROM registry.access.redhat.com/ubi9/ubi:latest" in images/Dockerfile.platform) so the Dockerfile and images/Dockerfile.platform reference concrete digests instead of tags; update ARG BASE_IMAGE to its digest form (or hardcode the digest in the FROM) and verify digests match your CI registry and rebuild/test images.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile`:
- Around line 43-47: The /tmp/helm-home directory is created root-owned with
chmod 755 so a non-root prow UID can't write to Helm state; update the
Dockerfile so that after creating /tmp/helm-home (the RUN mkdir -p ... && chmod
755 ... block) you chown it to the intended non-root user/UID (or create a
dedicated helm user and chown), and ensure you switch to that non-root USER
before runtime; keep the HELM_* env vars (HELM_CACHE_HOME, HELM_CONFIG_HOME,
HELM_DATA_HOME, HELM_PLUGINS) but make sure /tmp/helm-home and its
.cache/.config/.local/share subdirs are owned by the non-root UID (or are
group-writable to that UID) so Helm can write state at runtime.
---
Outside diff comments:
In `@Dockerfile`:
- Around line 32-86: The image currently sets USER root and never drops
privileges; create a non-root runtime user (e.g., UID 1000, group wheel or
nobody) and switch to it before the final ENTRYPOINT so the container does not
run as root. Ensure all files and directories used at runtime (references:
/usr/local/bin/hyperfleet-e2e, /usr/local/bin/hyperfleet-credential-provider,
/e2e, /e2e/configs, /e2e/testdata, /e2e/deploy-scripts, /e2e/env, /e2e/scripts)
are owned or accessible by that user (chown/chmod in the Dockerfile during image
build) and then add USER <nonroot> right before ENTRYPOINT to enforce the
non-root runtime. Maintain existing ENTRYPOINT and CMD but run them under the
non-root account.
- Line 7: Pin the base images to immutable digests: replace each floating tag
with the corresponding `@sha256` digest (e.g., change "FROM golang:1.25", "FROM
registry.ci.openshift.org/ci/hyperfleet-credential-provider:latest", the
ARG/default "ARG BASE_IMAGE=registry.access.redhat.com/ubi9/go-toolset" and the
later "FROM ${BASE_IMAGE}", and "FROM
registry.access.redhat.com/ubi9/ubi:latest" in images/Dockerfile.platform) so
the Dockerfile and images/Dockerfile.platform reference concrete digests instead
of tags; update ARG BASE_IMAGE to its digest form (or hardcode the digest in the
FROM) and verify digests match your CI registry and rebuild/test images.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d2e24a0c-9472-425c-b5d5-ff67acf818ea
📒 Files selected for processing (1)
Dockerfile
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
| RUN mkdir -p /tmp/helm-home && chmod 755 /tmp/helm-home | ||
| ENV HELM_CACHE_HOME=/tmp/helm-home/.cache/helm \ | ||
| HELM_CONFIG_HOME=/tmp/helm-home/.config/helm \ | ||
| HELM_DATA_HOME=/tmp/helm-home/.local/share/helm \ | ||
| HELM_PLUGINS=/usr/local/share/helm/plugins |
There was a problem hiding this comment.
/tmp/helm-home is not writable for prow’s non-root UID (CWE-732), so Helm state can still fail at runtime.
RUN mkdir -p /tmp/helm-home && chmod 755 /tmp/helm-home creates a root-owned directory that arbitrary non-root users cannot write to. That conflicts with the PR’s goal of making Helm usable when tests run non-root.
Suggested fix
-RUN mkdir -p /tmp/helm-home && chmod 755 /tmp/helm-home
+RUN mkdir -p /tmp/helm-home && \
+ chgrp -R 0 /tmp/helm-home && \
+ chmod -R g=u /tmp/helm-homeAs per coding guidelines, “Do not run as root — use USER with a non-root UID” and container permissions must avoid insecure/broken access patterns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Dockerfile` around lines 43 - 47, The /tmp/helm-home directory is created
root-owned with chmod 755 so a non-root prow UID can't write to Helm state;
update the Dockerfile so that after creating /tmp/helm-home (the RUN mkdir -p
... && chmod 755 ... block) you chown it to the intended non-root user/UID (or
create a dedicated helm user and chown), and ensure you switch to that non-root
USER before runtime; keep the HELM_* env vars (HELM_CACHE_HOME,
HELM_CONFIG_HOME, HELM_DATA_HOME, HELM_PLUGINS) but make sure /tmp/helm-home and
its .cache/.config/.local/share subdirs are owned by the non-root UID (or are
group-writable to that UID) so Helm can write state at runtime.
Source: Coding guidelines
|
/retest |
1 participant
Summary
Update e2e dockerfile to override default helm default directories. Seems like another limitation of prow since we are running the tests as non-root we don't have access to the $HOME directory.
Relates to HYPERFLEET-1057
Test Plan